Skip to content

Fix #15183 Syntax error with -!! - #9005

Open
chrchr-github wants to merge 2 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_15183
Open

chrchr-github wants to merge 2 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_15183

Conversation

@chrchr-github

Copy link
Copy Markdown
Collaborator

No description provided.

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

The fix works for ! and ~ after a unary +/-. It is incomplete, though: the same check in concatenateNegativeNumberAndAnyPositive() still reports a false syntaxError on valid code when another prefix operator follows the sign. Examples are return - --i;, return - ++i; and return +&x;. They are the same kind of bug as #15183 and fail the same way on base and on the PR. I found no regressions: invalid code such as return -!; is still rejected, now as internalAstError. return -~; is now accepted silently, but return ~; and return x + ~; are already accepted on base, so that is not new.

Testing

  • I built cppcheck and testrunner in pr/, and cppcheck in base/.
  • ./testrunner TestTokenizer: 392 tests, 0 failures.
  • I compared base and PR on small C files of the form int x; int f(int i) { <stmt> }, run with cppcheck -q --enable=all.
  • Fixed by the PR. Base reported syntaxError on each of these and the PR reports nothing wrong:
    • return -!!i;
    • return -~i;
    • return +!i;
    • return x * -!i;
    • return x == -!i;
    • return x < -~i;
    • return i ? -!i : -~i;
    • int a[] = {-!i, +~i};
    • case -!0:
    • return sizeof -!i;
  • Still a false syntax error on valid code, the same on base and PR:
int x; int f(int i) { return - --i; }
int x; int f(int i) { return - ++i; }
int x; int f(int i) { return +&x; }
$ ./cppcheck -q u.c
u.c:1:23: error: syntax error [syntaxError]
  • Invalid code:
    • return -!; now gives internalAstError; base gave syntaxError.
    • return -~; is now accepted without any error. Base also accepts return ~; and return x + ~;, so this is not a new kind of gap.

Automated review of a05990f by Claude. The pull request was built and tested locally.

Comment thread lib/tokenize.cpp Outdated
continue;

if (!tok->tokAt(2) || (tok->tokAt(2)->isOp() && !Token::Match(tok->tokAt(2), "[+-*]")))
if (!tok->tokAt(2) || (tok->tokAt(2)->isOp() && !Token::Match(tok->tokAt(2), "[+-*!~]")))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

Valid code with other prefix operators after a unary +/- still gets a false syntaxError here, for the same reason as #15183:

int x; int f(int i) { return - --i; }
int x; int f(int i) { return - ++i; }
int x; int f(int i) { return +&x; }

All three still give error: syntax error [syntaxError] with this PR. ++/-- are inc/dec op tokens, and & is an address-of operator here. Consider also allowing ++|--|& after the sign, or only rejecting operators that can't be unary.

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

I found no new problems. The PR now accepts !, ~, &, ++ and -- after a unary +/-. That fixes #15183 and the cases raised in the earlier review (- --i, - ++i, +&x), and the new tokenizer test covers them. Code that is still invalid, such as return -!;, return - ++; and return -!-;, is still rejected, now as internalAstError or syntaxError. One kind of invalid code is newly accepted without an error: return -&;. Base already accepts return &; and return x + &;, so this is the same known gap as -~; from the earlier review and not a new problem.

Earlier comments

# Comment By Where Status
1 False syntaxError still reported for - --i, - ++i, +&x AI review lib/tokenize.cpp:3781 ✅ Fixed

Testing

  • I built cppcheck and testrunner in pr/, and cppcheck in base/.
  • ./testrunner TestTokenizer: 392 tests, 0 failures. ./testrunner TestGarbage TestSimplifyTokens TestSymbolDatabase TestValueFlow: 0 failures.
  • I ran cppcheck -q --enable=all on 20 small files of the form int x; int f(int i) { <stmt> }.
  • Valid code, no syntax error now:
    • -!!i, - --i, +&x
    • x * +~i, x * + !i
    • - - -i, x - - - !i, - *p, -&x == 0
    • -!i ? -~i : +!i, x = -!-!-~i
  • Invalid code, compared with base:
Code Base PR
return -!; syntaxError internalAstError
return - ++; syntaxError internalAstError
return -!-; syntaxError syntaxError
return -&; syntaxError no error
return &; no error no error
return x + &; no error no error
return x * &; no error no error

Base already accepts the last three, so the PR does not create a new kind of gap.

  • The GCC extension return -&&lbl; gives syntaxError on both base and the PR.

Automated review of f623f9a by Claude. The pull request was built and tested locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants